-
-
Notifications
You must be signed in to change notification settings - Fork 379
add Apple operating system version #4253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
Why is this not moving forward? |
spec/version.dd
Outdated
$(TROW $(ARGS $(D Win32)) , $(ARGS Microsoft 32-bit Windows systems)) | ||
$(TROW $(ARGS $(D Win64)) , $(ARGS Microsoft 64-bit Windows systems)) | ||
$(TROW $(ARGS $(D linux)) , $(ARGS All Linux systems)) | ||
$(TROW $(ARGS $(D Apple)) , $(ARGS Apple systems OSX, iOS, TVOS, WatchOs and VisionOS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's supposed to be WatchOS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if we want these to list the OSes by their current name rather than the version identifiers, then it should arguably be macOS instead of OSX like it currently says next to the description for the OSX
version identifier. Though I don't really care much personally.
Aside from the typo, I'd merge this right now, but the dmd one hasn't been merged, and I don't know if I should be making that kind of call for dmd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe here should be:
$(TROW $(ARGS $(D Apple)) , $(ARGS Apple systems $(D OSX), $(D iOS), $(D TVOS), $(D WatchOS) and $(D VisionOS)))
As in the description after 'systems' some version identifiers are mentioned.
Same is relevant for row about Windows:
$(TROW $(ARGS $(D Windows)) , $(ARGS Microsoft Windows systems $(D Win32) and $(D Win64)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyrusmsk Mind doing a follow-up PR once this one is merged? Or do you think it would be important to have the markup changes from the start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could be a separate commit. I will check all other versions as well 👍
Well, presumably, this PR hasn't been merged, because the dmd one hasn't been, but I don't know why that one hasn't been merged. I don't usually get involved with dmd PRs. |
See dlang/dmd#21471 for implementation and rationale.